-
Notifications
You must be signed in to change notification settings - Fork 215
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add core promise-based typeset and convert functions, and simplify those in Startup. #1231
base: update/node-workers
Are you sure you want to change the base?
Conversation
…dd tests for that
…unneeded promises from startup.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left one comment, where I am trying to understand if there is a potential for a race conditions. I think it is fine, at least so far I could not come up with a scenario that would produce one.
// Cache old _readyPromise and replace it with a resolved | ||
// promise in case action() calls whenReady(), so we don't get | ||
// a circular dependency where the action is waiting on itself. | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get a race condition here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you are concerned about the caching and restoring of the _readyPromise
in lines 944, 945, and 953 below.
Here's how I understand the situation. First, ignore the machine for a moment, and just think about this as
public whenReady(action: () => any): Promise<any> {
return (this._readyPromise = this._readyPromise.then(() => action());
}
which is what it would be without the caching of the promise. In that case, as long as action()
doesn't call a function that calls whenReady()
, I think you will agree that this will serialize the calls to whenReady()
. (The _readyPromise
property is protected
and the only function that changes it is whenReady()
.) So a question only arrises when action()
calls a function that itself uses whenReady()
.
Note that if multiple whenReady()
calls are made, the actions will be queued, and not executed immediately. For example, if md
is a MathDocument
md.whenReady(() => console.log('A'));
md.whenReady(() => console.log('B'));
md.whenReady(() => console.log('C'));
console.log('queued');
will produce
queued
A
B
C
If the action()
creates a promise, it should return that promise so that whenReady()
will in term wait for that promise to complete. So, for example
md.whenReady(() => console.log('A'));
md.whenReady(() => new Promise((ok, fail) => setTimeout(() => {console.log('B'); ok()}, 1000)));
md.whenReady(() => console.log('C'));
console.log('queued');
will produce the same output as above, but with a 1 second wait before the B
is produced.
Similarly, for an asynchronous function that uses await
, which returns a promise automatically, whenReady()
will wait for the async function to finish. Thus
const promise = new Promise((ok, fail) => setTimeout(ok, 1000));
md.whenReady(() => console.log('A'));
md.whenReady(async () => {await promise; console.log('B')});
md.whenReady(() => console.log('C'));
console.log('queued');
produces the same results. So as long as action()
returns the promise it creates, all is well.
The complication comes when action()
calls a function that itself uses whenReady()
(or calls it directly). Consider, for example:
md.whenReady(() => console.log('A'));
md.whenReady(() => md.whenReady(() => console.log('B')));
md.whenReady(() => console.log('C'));
console.log('queued');
Here, we get the queued
message and the A
, but nothing else. In this case when the queued
message is produced, all three actions have been but none of them performed, yet, and the _readyPromise
is the one that resolves when the third action is complete (when console.log('C')
is performed).
At this point, the _readyPromise
looks like
Promise.resolve()
.then(() => console.log('A'))
.then(() => md.whenReady(() => console.log('B')))
.then(() => console.log('C'));
and none of the then
actions have been taken. Now when javascript becomes idle, the first then
is executed, and the A
is printed and its promise is resolved, so the next then
can be performed.
At this point, the inner whenReady()
function is called, and adds is action to then
for the _readyPromise
, so at the end of the list above, and then returns that promise to that second then
in the list above. That means the then
will wait for that promise to complete before it resolves; but because that promise follows the last one in the list above, it will not resolve until the previous ones do. That means we are in a catch-22 and the list will stop at this point with no additional actions ever being performed.
It is that situation that is being handled by adding the caching of the _readyPromise
. With that in place, each of the then
functions above first cache the promise, replace it with Promise.resolve()
, call the action currently listed in the then
saving its result, puts back the promise, and returns the saved result. This has no effect on the first and third actions, but the middle one is changed so that when the whenReady()
function runs, rather than adding its action to the end of the current list, it adds its action to a new Promise.resolve()
and returns that. That is, the second action now effectively returns
Promise.resolve().then(() => console.log('B'));
That is the value saved as the result
at line 949, and then the _readyPromise
is returned to its original value. Because this result is a promise, the second then
in the list above will wait for that promise to resolve before it is resolved itself. That means that when javascript is next idle, the console.log('B')
will be performed, then the promise listed just above from the whenReady()
will resolve, and that allows the second then
to resolve. That means the final then
can be performed, allowing the console.log('C')
to execute. This effectively breaks the lock-out situation that occurred originally.
Because the cached _readyPromise
is put back directly after calling action()
, and because no idle tasks can be performed between saving the promise and putting it back (only the top-level code from action()
can run, not any of its promises), no race condition exists, and any other whenReady()
functions that aren't within the action()
will find the _readyPromise
to be as though it had never been changed. And as long as the action()
function returns any promises that it receives from whenReady()
calls (or the promises from other functions that call whenReady()
), the actions will be properly serialized. Note also that whenReady()
calls can be nested to any depth with this strategy.
If action()
fails to return a promise, as, for example, with
md.whenReady(() => console.log('A'));
md.whenReady(() => {md.whenReady(() => console.log('B'))});
md.whenReady(() => console.log('C'));
console.log('queued');
where the second then
doesn't return the promise from the nested whenReady()
call, then console.log('C')
will run before console.log('B')
, since the former was added to its then
before the latter was added to the Promise.resolve()
that replaced the _readyPromise
chain, and these two are not tied together by a common promise chain. That could cause a potential intermingling of the actions. I would consider not returning the promise from whenReady()
to be a bug in the action()
code for that reason.
While writing this, I realized that there is a way to avoid even that issue. Changing lines 950 through 960 to
//
// Get a promise that returns the result after
// any new _readyPromise resolves (in case action
// called whenReady() or another function that does).
//
const promise = this._readyPromise.then(() => result);
//
// Put back the original promise.
//
this._readyPromise = ready;
//
// Return promise that returns the result. The original
// _readyPromise will wait on it to complete before it resolves,
// since promises that return promises automatically chain.
// This inserts any new _readyPromise promises into the
// original _readyPromise chain at this point.
//
return promise;
would make sure that any new _readyPromise
promises are resolved before the result gets returned. That effectively injects any new _readyPromise
chain into the original chain at the current location rather than at the end, thereby avoiding the lock-out situation that we saw above.
I will make a new commit for that change.
Now that both speech and output (when font data is loaded) use promises and retries, it seems appropriate to add promise-based calls to the core MathDocument for typesetting and conversion, like the ones in the Startup module.
This PR adds those functions. In particular, new
renderPromise()
,rerenderPromise()
, andconvertPromise()
methods are added to theMathDocument
class, and the Startup module now takes advantage of these when creatingMathJax.typesetPromise()
and the various conversion functions likeMathJax.tex2chtmlPromise()
.The old
MathJax.typesetPromise()
and conversion functions had been modified to use theMathJax.startup.promise
to make sure that they operated serially. The new MathDocument methods now do that using a new_readyPromise
, and a newwhenReady()
method is added to the MathDocument to make that easier. This function takes an action to be performed when the current_readyPromise
has been resolved, and resets that promise to a new promise that resolves when the action is complete. (This is effectively makes a queue of actions to be performed serially, with actions that return promises causing the following actions to wait until the earlier ones complete.) Some care is taken to prevent a circular dependency where the action performed bywhenReady()
tries to wait on the promise that it is supposed to resolve. (See the comments in that function.) That way, even thoughrenderPromise()
useswhenReady()
itself, you can still callrenderPromise()
from within an action that appears within anotherwhenReady()
call, as is done in some of the functions created by the Startup module.The
handleRetriesFor()
function, which handles promises created by loading of extensions, is extended to handle async functions, since those are used in the new promise-based rendering and conversion functions. New tests are added to cover this case in theRetries.test.ts
file.All of the promise-based functions wait not just for retry promises, but also for any promises created during processing of the document's renderActions (like those created for handling the speech creation). The variable use to store these has been made private and renamed
_actionPromises
, and service routinessavePromise()
,clearPromises()
andactionPromises()
have been added in order to manipulate the list of saved promises, rather than dealing with the promise array directly. So the speech component callsdocument.savePromise()
rather thandocument.renderPromises.push()
as it used to.The Startup module has been updated to take advantage of the new promise-based code of the document. In the past, it used its own
Startup.promise
to serialize the promise-based calls, but it now uses that only to indicate when the initial typesetting is complete, and falls back on thewhenReady()
function to handle the serialization. It also used to have arerenderPromise
that was used for the menu code when a re-render action was necessary when a component was loaded (it is only needed of anything has been typset already, so no re-rendering needs to be done during startup when the menu may be loading user-selected components.) This promise has been replaced by a booleanhasTypeset
indicating that typesetting has occurred, and a rerender is needed.